-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Transform for F# Pipeline #1
WIP: Transform for F# Pipeline #1
Conversation
Arrow functions don't require parens, so we def need tests to make sure those are transpiled correctly as well. |
packages/babel-plugin-proposal-pipeline-operator/src/fsharpVisitor.js
Outdated
Show resolved
Hide resolved
packages/babel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/await/input.js
Show resolved
Hide resolved
Yeah, that would be nice. I'll add some tests on that vein for integration testing purposes. But this is more of a parser concern, in my opinion. It would be nice to cover that more thoroughly in the parser tests. |
I think we've got it covered there, but I'd definitely be more comfortable if I saw the full transform here. We can then address that in the parser if we need to. |
Trying to write parenless tests. What should const y = 2;
const g = (x) => x
|> (y) => (
y + 1
|> (z) => z * y
) |
Yeah, Nicolo pointed out the same issue on the parser PR. I think that's a bug I'll need to address; in the last arrow, |
Oh wait, you're writing this with parens... so that's a different issue. |
Ping me on slack to discuss this, probably easier that way. |
Yeah. I stole Nicolo's code from there. 😜 |
For anyone following this, we've discussed it and came to the conclusion that |
6738d16
to
523e18c
Compare
Inludes support for optimizing single-parameter arrow functions
523e18c
to
bbb8a14
Compare
Updated to latest parser and did the initial clean up. I will port this PR to the main babel repo once the parser PR gets merged. I still plan on deduping the arrow function optimization logic between this and minimal.
Regarding that, do you think the added test is sufficient to address the concerns you raised? |
Yeah, this looks great! Thank you so much for working on this. This is finally happeninggg! Makes me really excited for the possibilities of the proposal itself. |
Deduped optimization logic between fsharp and minimal. Please let me know what you think. |
Oops... Fat-fingered the PR branch. Reopening... |
LGTM! Looking forward to getting this all in place. |
const fsharpVisitor = { | ||
BinaryExpression(path) { | ||
const { scope } = path; | ||
const { node } = path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: these two statements can be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in ecbb465
call, | ||
]); | ||
path.replaceWith(sequence); | ||
maybeOptimizePipelineSequence(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of creating a temporary path for the sequence expression and then optimizing it away, would it be possible to do something like this?
path.replaceWith(
buildOptimizedSequenceExpression({
assign: t.assignmentExpression("=", t.cloneNode(placeholder), left),
call,
path,
})
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started with something similar to this but ended up passing the path in because I needed it for the scope.rename
call. I think your approach is doable and would be cleaner. I'll give it a try.
y + 1 | ||
|> (z) => z * y); | ||
} | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this random indentation makes it really hard to review this file 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is hard for me too :-p
I'm still getting the hang of how to indent pipelines. I'll see what I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see 4811f08 and let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah they look better. As a general rule, I think that if the pipeline body is multiline, )
should aligned with the |
of |>
and there should be a line break after (
. SImilar to
return (
x => y
);
Let's hope Prettier implements them soon 😛
packages/babel-plugin-proposal-pipeline-operator/test/fixtures/fsharp/arrow-functions/input.js
Show resolved
Hide resolved
|
||
var inc = x => x + 1; | ||
|
||
expect((_ = 10, inc(_))).toBe(11); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If at least one of the following conditions is true, we could optimize it to inc(10)
:
- the right expression is an identifier and it is declared
- the left expression is immutable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also applies to minimal. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this in 63b7c04. I'm marking the PR as WIP, though. I'm uncomfortable with buildOptimizedSequenceExpression
now no longer returning a sequence expression. And it also does some AST deconstructing that I would like to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you prefer, we can work on this optimization in a separate PR, and keep the current one as it was.
This was moved to the main repo as babel#9984 |
This is a transform for the F# proposal of the pipeline operator based on @mAAdhaTTah's parsing code.
It is basically the minimal visitor with support for paramless await. The most relevant lines are fsharpVisitor.js/44-47
TODO: